Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upadte 2d20 roll and damage roll #380

Closed
wants to merge 15 commits into from
Closed

Conversation

bb46003
Copy link
Contributor

@bb46003 bb46003 commented May 14, 2024

change for 2d20 roll :

  • add automatization for buing d20 dice including PC and GM, allow to buy dice for GM AP
  • add button for assistance, always will roll 1d20
  • reduce ammo for GM (option i settings)
  • reduce 10 amo for gatling guns

change for damage roll :

  • for guns fire rate can be add at begining of rolling, read weapon property and chcek if we have enough amo to shoot more bullet
  • for mele/unarmed can spend max 3 AP for PC and GM
  • reduce AP if avaliable for both PC and GM
  • for PC buy from GM, with notification, add to GM AP automaticaly
  • add button from chat read weapon fire rate and chcek avaliable amo

bb46003 and others added 4 commits February 24, 2024 16:39
upadate to noew dev baranch
change for 2d20 roll :
 - add automatization for buing d20 dice including PC and GM, allow to buy dice for GM AP
- add button for assistance, always will roll 1d20
- reduce ammo for GM (option i settings)
- reduce 10 amo for gatling guns
change for damage roll :
- for guns fire rate can be add at begining of rolling, read weapon property and chcek if we have enough amo to shoot more bullet
- for mele/unarmed can spend max 3 AP for PC and GM
- reduce AP if avaliable for both PC and GM
- for PC buy from GM, with notification, add to GM AP automaticaly 
- add button from chat read weapon fire rate and chcek avaliable amo
@Muttley
Copy link
Owner

Muttley commented May 14, 2024

I'll try and take a look at this after the next release. I think it looks like it might need a bit of beautification before it can go into a release, judging by the screenshots you've shared.

@bb46003
Copy link
Contributor Author

bb46003 commented May 14, 2024

I was not spending much time on css just to look reasonable fine

i18n/pl.yaml Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't check in changes to the non en.yaml translation files, any changes will be overwritten with the next sync from Crowdin anyway.

i18n/en.yaml Outdated
Comment on lines 494 to 496
FALLOUT.UI.Additionalamo: Additional damage dice from firerate
FALLOUT.UI.Additionalmeledmg: Additional damage dice for party AP
FALLOUT.UI.AdditionalmeledmgOverseer: Additional damage dice for overseer AP
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in your other pull request, please try and keep keys readable. Going forward I'm trying to standardise the i18n key style to snake_case, avoiding unecessary abbreviations where possible

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several instances in this file where function/method names are hard to read being all lower-case with multiple words merged together.

Please make sure all function/methods names in camelCaseFormat.

Also, any new html being rendered should be done via relevant template files, not embedded in code please. I'm gradually stripping out existing instances of that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are similar problems in this file as mentioned in the comment for Dialog2d20.mjs

@@ -132,7 +132,7 @@ export default class FalloutChat {
const falloutRoll = message.flags.falloutroll;
const weapon = message.flags.weapon;

fallout.DialogD6.createDialog({
fallout.DialogD6.addcreateDialog({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method names should be camelCaseStyle

Copy link
Owner

@Muttley Muttley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid you'll need to resolve any problems commented on in this code before I can safely merge it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the manifest config settings. Please don't make changes to system.json

@bb46003
Copy link
Contributor Author

bb46003 commented Jun 6, 2024

I do all mentioned request, as well fixing few issue that I recognie like macros for rollind d6 or 2d20

@bb46003 bb46003 requested a review from Muttley June 6, 2024 09:41
@Muttley
Copy link
Owner

Muttley commented Jun 16, 2024

Having had a play around with this, I really don't like the workflow changes this makes to the dice rolling, as it feels quite clunky and unintuitive.

I'm afraid I'm going to have to reject this one as it would be more work to resolve the issues with this implementation than it would be to write the new dice roller I'd already envisioned replacing the existing one.

@Muttley Muttley closed this Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants